Should this be a required input instead of defaulting to Active?

What does it mean for multiple ClusterExtensionRevision objects for the same ClusterExtension to be Active?

Active is the current one, Archive are the old states.
CER are like snapshot of the CE in certain period of time

So there can never be more than 1 CER marked as Active?

Also, if this field is required, it shouldn't be defaulted. Required with a default is contradictory.

During an upgrade, while a new ClusterExtensionRevision is being rolled out and before the old revision has begun to be torn down, both CERs will be marked as Active.

I'm in agreement on the defaulting - plus when we create revisions we should be setting it explicitly anyway rather than relying on defaulting.

Please make sure there is a clear description of what each state means and what it means if you have multiple instances of the Active state.

Not used in the API as an allowed value?

'Pauses' will be removed for now. Even through the underlying library supports it, it's not needed rn.

We discourage the usage of the pattern marker because of the lack of helpful error messages that are returned to end-users (a regex string instead of a sentence explaining the constraints).

Instead, use a CEL expression that enforces this constraint and returns a helpful message like:

To check, what is the minimum KAS version this API is intended to be installed upon. For 1.35 onwards we will prefer format: k8s-short-name. From 1.32 you can use the formats library. If you're supporting versions older than that you'll have to use the regex

TIL about the new format :)

TIL here as well - we should use format: k8s-short-name instead. I don't think this API will be active on anything less than 1.32, @perdasilva correct me if I'm wrong?

Explicitly mark the field as required.

Is the empty string a valid value?

Document the uniqueness constraint keyed on the phase name in the GoDoc.

Could you please explain this one?
I did not get the request

The +listType=map and +listMapKey=name markers enforce a uniqueness constraint on this field based on the name of the phase.

Markers are not included in generated documentation so we strongly recommend making all the input constraints explicitly in the GoDoc. I do not see in the GoDoc that would be used in generated documentation for the field mentioning that phase names must be unique, so I'm asking that it be added.

Nit: We normally try to follow a pattern where we explicitly introduce the allowed values as well with a sentence like:

Allowed values are Prevent, IfNoController, ...

before going into the When ... sections.

Might be helpful context, could you elaborate for me why you want this field to be optional?

Leaving this field as optional helps protect against accidental misuse; the field needs to be one of these three, and requiring every user to choose one of them opens more opportunities to cause problems from controller conflicts.

Who is it that is creating this resource? My interpretation was that OLM itself would be creating these resources and not end-users - is that incorrect?

Yes that's correct, and a human shouldn't need to interact with this API apart from some kind of manual intervention. IMO this also helps for the non-human users, in the sense that it may help prevent bugs that create controller vs controller conflicts.

I'd recommend avoiding defaulting like this when it is a controller that is responsible for creating these resources - it obfuscates whether or not the controller intentionally set a value or if it omitted a value and it defaulted to that value.

If the field is effectively required, make it the responsibility of your controller to always set this value and have your default behavior explicitly encoded in the controller.

Do you ever intend to modify the default behavior? Done this way, the defaulting logic becomes a part of your API contract.

I personally can't imagine us changing the defaulting behavior away from Prevent, which is the safest option of the three.

I could be wrong here, but IIRC most embedded resource types will use https://pkg.go.dev/k8s.io/kubernetes/pkg/runtime#RawExtension as the type there.

I don't have a strong opinion here though as I've not reviewed many APIs that are embedding another resource blob within them. If this is working, 👍

v1? Have you shipped this API already? Typically, APIs for experimental type features will start as v1alpha1. In OpenShift, it is expected that TP APIs are v1alphaN until ready to be promoted.

This seems like Available = False might be more appropriate?

I believe the convention we're trying to set here is that True vs False implies we're actively checking readiness probes to determine the state of installed content. Once a revision has been Archived, we are no longer checking those probes, thus the availability is Unknown. Technically, the resources could still be available if they haven't been completely garbage collected yet, but since this resource is Archived we're no longer paying attention to its resources.

Gotcha - maybe we can do some updating of the text here to clarify that the unknown status is because it is archived and the system is no longer paying attention to the resources associated with this, as opposed to the current wording where it sounds like it is because the resources associated with it are removed?